-
-
Notifications
You must be signed in to change notification settings - Fork 664
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
go_test: Allow not registering a SIGTERM handler #3827
base: master
Are you sure you want to change the base?
go_test: Allow not registering a SIGTERM handler #3827
Conversation
98260b7
to
f808fd7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cc @linzhp Would this help with your tests?
This helps, but we simply add |
The rules_go SIGTERM handler causes tests which themselves test responding to SIGTERM to panic and fail.
f808fd7
to
f100aec
Compare
Ooh that's a nice idea! Yeah, this approach works, and it seems reasonable (though a little noisy) for tests which are explicitly handling signals themselves, but it feels much noisier to have to add to all tests if they use |
goleak will come with a default ignorelist entry in the future as far as I understood @linzhp. I do prefer the Go-only solution in that case if we document it. |
Good point on goleak. Uber maintains a internal patch of all known leaks, but there is no plan to put the list in the open source goleak project. It's unfair to ask every user of goleak to maintain a patch. So in combination of the SIGTERM reset and goleak opt-out, this PR is quite useful, unless we have better implementation of the timeout handler. |
The more I think about it, the less I think this new attribute provides a good user experience: It's used to work around two separate issues, both of which would better be fixed in other layers. For the few tests that require bespoke SIGTERM handling, the solution of manually resetting the handler from Go seems much more targeted and discoverable. It also communicates the intent better and guards against other conflicting handlers other libraries may register in For goleak, disabling the new timeout handler just to avoid the false positive is also not great as it means users have to choose between using goleak and having better timeout reporting. @linzhp Would goleak be open to contributions to improve this interaction? |
I am not a maintainer of goleak, based on this reply, they don't want a default ignore list, given that goleak supports IgnoreAnyFunction. So tests that uses goleak can call func TestMain(m *testing.M) {
goleak.VerifyTestMain(m, goleak.IgnoreAnyFunction("github.com/bazelbuild/rules_go/go/tools/bzltestutil.RegisterTimeoutHandler.func1"))
} |
rules_go added a SIGTERM handler that has a goroutine that survives the scope of the goleak check. Currently, the best known workaround is to ignore this goroutine. uber-go/goleak#119 bazel-contrib/rules_go#3749 bazel-contrib/rules_go#3827 (comment)
rules_go added a SIGTERM handler that has a goroutine that survives the scope of the goleak check. Currently, the best known workaround is to ignore this goroutine. uber-go/goleak#119 bazel-contrib/rules_go#3749 bazel-contrib/rules_go#3827 (comment)
rules_go added a SIGTERM handler that has a goroutine that survives the scope of the goleak check. Currently, the best known workaround is to ignore this goroutine. uber-go/goleak#119 bazel-contrib/rules_go#3749 bazel-contrib/rules_go#3827 (comment)
I followed up on the goleak thread, maybe we can solve this by introducing an ignore mechanism that rules_go can use and goleak doesn't have to maintain. |
rules_go added a SIGTERM handler that has a goroutine that survives the scope of the goleak check. Currently, the best known workaround is to ignore this goroutine. uber-go/goleak#119 bazel-contrib/rules_go#3749 bazel-contrib/rules_go#3827 (comment)
What type of PR is this?
Bug fix
Feature
What does this PR do? Why is it needed?
The rules_go SIGTERM handler causes tests which themselves test
responding to SIGTERM to panic and fail.